Add tests for the behavior of JSON.generate with base types subclasses #679
Conversation
|
|
||
| def test_array_subclass_with_to_s | ||
| assert_equal '[]', JSON.generate(ArrayWithToS.new) | ||
| assert_equal '{"JSONGeneratorTest::ArrayWithToS#to_s":1}', JSON.generate(ArrayWithToS.new => 1) |
There was a problem hiding this comment.
This doesn't seem reasonable, i.e. calling to_s on an Array subclass (both semantically and performance-wise).
Inheriting from core classes is frowned upon, so without a convincing use case I see no value to respect those old weird/broken semantics.
There was a problem hiding this comment.
Yeah, I'm not sold yet on which behavior makes the most sense, I just want to make sure it's tested and that all implementation behave the same, and then think about what makes sense.
There was a problem hiding this comment.
I think ideally we wouldn't treat subclasses specially at all, it doesn't seem reasonable to subclass a core type and then expect different behavior than the superclass for JSON behavior and many other things (Liskov substitution principle).
If someone wants to have some custom serialization to JSON, it seems simple enough to have a custom type not inheriting from a core type and having to_json implemented on it.
There was a problem hiding this comment.
I'm not disagreeing, I'm just saying we should be very deliberate about it, because backward compatibility is very important.
There was a problem hiding this comment.
Ah for this specific example I missed that to_s is only called because the ArrayWithToS is a Hash key, that seems less crazy at least 😅
|
For the ActiveSupport use case in #667 maybe this extra escaping should/could be a JSON.dump option? Then maybe we wouldn't need to treat subclasses any specially. |
|
I created a self contained test case to more easily test historical behavior, and it seems we already diverged: https://gist.github.com/casperisfine/a379245862b0d5273086e5053657460a I'll update the test suite to have the same behavior as 2.7.2 |
casperisfine
force-pushed
the
generator-custom-base-types
branch
from
74946aa
to
499f3a9
Compare
casperisfine
force-pushed
the
generator-custom-base-types
branch
from
499f3a9
to
614921d
Compare
| if (RB_LIKELY(RBASIC_CLASS(key) == rb_cString)) { | ||
| key_to_s = key; | ||
| } else { | ||
| key_to_s = rb_funcall(key, i_to_s, 0); | ||
| } |
There was a problem hiding this comment.
We had regressed here on the C implementation.
…ypes Add tests for the behavior of JSON.generate with base types subclasses
Ref: #674
Ref: #668
The behavior on such case it quite unclear, the goal here is to figure out whatever was the behavior on Cext version of
json 2.7.0and get all implementations to converge.We can then decide to make them all behave differently if we so wish.
FYI: @eregon